feat(autoware_utils): porting from autoware_universe_utils #23

JianKangEgon · 2025-01-16T07:28:32Z

Description

We are porting package autoware_universe_utils to autoware.core, and this PR adds the package to core.

Issue Link Port 'autoware_vehicle_info_utils' and 'autoware_universe_utils' from Autoware Universe to Autoware Core autoware.core#131

How was this PR tested?

Notes for reviewers

None.

Effects on system behavior

None.

update readme
chang function naming pattern
create pr

Signed-off-by: JianKangEgon <[email protected]>

Dev

github-actions · 2025-01-16T07:28:43Z

Thank you for contributing to the Autoware project!

🚧 If your pull request is in progress, switch it to draft mode.

Please ensure:

You've checked our contribution guidelines.
Your PR follows our pull request guidelines.
All required CI checks pass before marking the PR ready for review.

Signed-off-by: JianKangEgon <[email protected]>

… same namespace will cause compile error when triggering template specilization Signed-off-by: NorahXiong <[email protected]>

Signed-off-by: JianKangEgon <[email protected]>

youtalk

I think the PR title and description are not correct. Please update them properly first.

mitsudome-r · 2025-01-21T08:49:03Z

Hi, it's a bit hard to review since the PR is quite big. Is it possible to break it into pieces? Maybe breaking it into the following the include directory structure might be good:

geometry
math
ros
system
transform

mitsudome-r · 2025-01-21T12:36:37Z

Sorry, please ignore my comment above. I think we will be able to review it with the current size.

esteve

According to the coding conventions we have, code must live under a namespace like:

autoware::PACKAGE_NAME

@JianKangEgon I've added a bunch of suggestions that you can just accept.

esteve · 2025-01-21T13:56:00Z

autoware_utils/include/autoware_utils/geometry/alt_geometry.hpp

+#include <utility>
+#include <vector>
+
+namespace autoware_utils


The namespace for all the header files needs to be autoware::utils

Suggested change

namespace autoware_utils

namespace autoware::utils

esteve · 2025-01-21T13:56:43Z

autoware_utils/include/autoware_utils/geometry/boost_geometry.hpp

+
+#include <geometry_msgs/msg/point.hpp>
+
+namespace autoware_utils


Suggested change

namespace autoware_utils

namespace autoware::utils

esteve · 2025-01-21T13:56:55Z

autoware_utils/include/autoware_utils/geometry/boost_geometry.hpp

+  point.z() = msg.z;
+  return point;
+}
+}  // namespace autoware_utils


Suggested change

} // namespace autoware_utils

} // namespace autoware::utils

esteve · 2025-01-21T13:57:05Z

autoware_utils/include/autoware_utils/geometry/boost_polygon_utils.hpp

+
+#include <vector>
+
+namespace autoware_utils


Suggested change

namespace autoware_utils

namespace autoware::utils

esteve · 2025-01-21T13:57:13Z

autoware_utils/include/autoware_utils/geometry/boost_polygon_utils.hpp

+  const double base_to_rear, const double width);
+double get_area(const autoware_perception_msgs::msg::Shape & shape);
+Polygon2d expand_polygon(const Polygon2d & input_polygon, const double offset);
+}  // namespace autoware_utils


Suggested change

} // namespace autoware_utils

} // namespace autoware::utils

esteve · 2025-01-21T13:58:21Z

autoware_utils/include/autoware_utils/geometry/random_concave_polygon.hpp

+  const std::function<
+    bool(const autoware_utils::Polygon2d &, const autoware_utils::Polygon2d &)> &);
+
+}  // namespace autoware_utils


Suggested change

} // namespace autoware_utils

} // namespace autoware::utils

esteve · 2025-01-21T13:58:29Z

autoware_utils/include/autoware_utils/geometry/random_convex_polygon.hpp

+
+#include <autoware_utils/geometry/geometry.hpp>
+
+namespace autoware_utils


Suggested change

namespace autoware_utils

namespace autoware::utils

Thanks for reviewing， esteve san

we have change the name space according to your comment

Have a nice day！

esteve · 2025-01-21T13:58:40Z

autoware_utils/include/autoware_utils/geometry/random_convex_polygon.hpp

+/// @param max points will be generated in the range [-max,max]
+/// @details algorithm from https://cglab.ca/~sander/misc/ConvexGeneration/convex.html
+Polygon2d random_convex_polygon(const size_t vertices, const double max);
+}  // namespace autoware_utils


Suggested change

} // namespace autoware_utils

} // namespace autoware::utils

esteve · 2025-01-21T13:58:51Z

autoware_utils/include/autoware_utils/geometry/sat_2d.hpp

+
+#include "autoware_utils/geometry/boost_geometry.hpp"
+
+namespace autoware_utils::sat


Suggested change

namespace autoware_utils::sat

namespace autoware::utils::sat

esteve · 2025-01-21T13:58:59Z

autoware_utils/include/autoware_utils/geometry/sat_2d.hpp

+ */
+bool intersects(const Polygon2d & convex_polygon1, const Polygon2d & convex_polygon2);
+
+}  // namespace autoware_utils::sat


Suggested change

} // namespace autoware_utils::sat

} // namespace autoware::utils::sat

youtalk · 2025-01-22T00:58:33Z

autoware_utils/include/autoware_utils/autoware_utils.hpp

Including too many files has led to poor maintainability, and including autoware_utils.hpp results in significant dependencies.
Therefore, for example, how about creating autoware_utils/geometry.hpp, which consolidates the includes under autoware_utils/geometry, and including it in autoware_utils.hpp? This way, files that only need geometry will not be affected by other dependencies.

This is a porting PR, so this fix can be included in a separate PR.

Agree with you kondo san， @youtalk

Let‘s porting first then divide it in core repo in another pr

have a nice day

心刚

Signed-off-by: JianKangEgon <[email protected]>

mitsudome-r · 2025-01-22T05:44:46Z

@esteve @JianKangEgon

According to the coding conventions we have, code must live under a namespace like:
autoware::PACKAGE_NAME

I think this is a good point. However, this modification requires changes to different repositories that depend on this package as well (e.g., in autoware_lanelet2_extension, autoware.universe, etc.).

I suggest we can focus first on moving the function from autoware_universe_utils to autoware_utils in this PR, then do namespace refactoring in another PR. We probably have to change the directory structure as well.

…updated in further version Signed-off-by: JianKangEgon <[email protected]>

liuXinGangChina · 2025-01-22T08:28:41Z

@esteve @JianKangEgon

According to the coding conventions we have, code must live under a namespace like:
autoware::PACKAGE_NAME

I think this is a good point. However, this modification requires changes to different repositories that depend on this package as well (e.g., in autoware_lanelet2_extension, autoware.universe, etc.).

I suggest we can focus first on moving the function from autoware_universe_utils to autoware_utils in this PR, then do namespace refactoring in another PR. We probably have to change the directory structure as well.

Good afternoon, mits san @mitsudome-r

autoware_utils pr already revert, please help us review and merge it

Best regards
心刚

liuXinGangChina · 2025-01-22T08:49:18Z

I think the PR title and description are not correct. Please update them properly first.

Good afternoon Kondo San @youtalk

Thanks for reviewing this pr, can you indicate the issue in the title and description of the pr. I will inform the engineer to modify according to your comment

Best Regards

心刚

esteve · 2025-01-22T10:45:16Z

@esteve @JianKangEgon
I suggest we can focus first on moving the function from autoware_universe_utils to autoware_utils in this PR, then do namespace refactoring in another PR. We probably have to change the directory structure as well.

The respective packages in autoware.universe already have the autoware::PACKAGE_NAME convention (e.g. https://github.com/autowarefoundation/autoware.universe/blob/main/common/autoware_vehicle_info_utils/include/autoware_vehicle_info_utils/vehicle_info.hpp#L20). This PR is undoing that work, so we'll end up doing this three times: once when we made the change in autoware.universe, then undoing it here, and then another PR in the future to do it again.

I propose we just move the code and keep the convention that's already implemented in autoware_vehicle_info_utils, autoware_time_utils, etc. (for example https://github.com/autowarefoundation/autoware.universe/blob/main/common/autoware_time_utils/include/autoware/time_utils/time_utils.hpp)

liuXinGangChina · 2025-01-23T01:31:52Z

@esteve @JianKangEgon
I suggest we can focus first on moving the function from autoware_universe_utils to autoware_utils in this PR, then do namespace refactoring in another PR. We probably have to change the directory structure as well.

The respective packages in autoware.universe already have the autoware::PACKAGE_NAME convention (e.g. https://github.com/autowarefoundation/autoware.universe/blob/main/common/autoware_vehicle_info_utils/include/autoware_vehicle_info_utils/vehicle_info.hpp#L20). This PR is undoing that work, so we'll end up doing this three times: once when we made the change in autoware.universe, then undoing it here, and then another PR in the future to do it again.

I propose we just move the code and keep the convention that's already implemented in autoware_vehicle_info_utils, autoware_time_utils, etc. (for example https://github.com/autowarefoundation/autoware.universe/blob/main/common/autoware_time_utils/include/autoware/time_utils/time_utils.hpp)

I get your point , esteve san. @esteve

I have a meeting with mits san yesterday, and his idea is we merge this pr first keep "autoware_util::xxx" name space pattern and followed by a pr to change namespace to "autoware::xxx" along with the modifications of package under core repo which use autoware_utils

is this solution ok to you?

Best regards

心刚

youtalk

This is not autoware.core or autoware.universe, but the autoware_utils repository, so even if the namespace rules are different, it can still be somewhat understood.

I've changed the PR's title.

youtalk · 2025-01-23T06:14:24Z

@mitsudome-r Many PRs depend on this and can’t be merged, so although the issue with @esteve’s namespace remains, shall we proceed with a bypass merge?

mitsudome-r · 2025-01-23T07:18:02Z

@esteve @youtalk

I think we also have an issue that this package is already released as 1.0.0 in ROS Buildfarm. We might want to be careful with changing the API in the same distribution. We probably should do the namespace when we prepare the release for 2.0.0.

Since this is being a blocker for many other PRs, let's merge it and do the namespace renaming in another occasion.

mitsudome-r

The code looks okay with me.

esteve · 2025-01-23T08:54:54Z

This is not autoware.core or autoware.universe, but the autoware_utils repository

I was under the impression that the code conventions apply to all the projects in the autowarefoundation organization. @xmfcx @mitsudome-r Could you please clarify?

esteve · 2025-01-23T12:10:52Z

@JianKangEgon @mitsudome-r @youtalk @xmfcx my mistake, sorry, I thought that the autoware_utils repository was empty and that's why I suggested to copy the code as is from autoware.universe, but I see that there's already code in autoware_utils under the autoware_utils namespace, and I agree it's better to keep new code under the existing autoware_utils namespace and the same filesystem structure to avoid any confusion.

After all the PRs have been merged we can discuss whether we should follow the autoware.core and autoware.universe code conventions or keep it as is, but for now I'm fine with using the autoware_utils namespace.

esteve

@JianKangEgon LGTM, thanks!

JianKangEgon and others added 5 commits January 15, 2025 18:02

feat: PAWF-407 update autoware_utils & rename function in snake format

9fa4c0b

Signed-off-by: JianKangEgon <[email protected]>

feat: update README for autoware_utils

df8e23a

Signed-off-by: JianKangEgon <[email protected]>

feat: fix example code compile error

4609190

Signed-off-by: JianKangEgon <[email protected]>

style(pre-commit): autofix

e1e65c7

Merge pull request #1 from JianKangEgon/dev

54be0ce

Dev

JianKangEgon requested review from esteve, mitsudome-r and youtalk as code owners January 16, 2025 07:28

style(pre-commit): autofix

d4fa161

JianKangEgon self-assigned this Jan 16, 2025

JianKangEgon added the run:build-and-test-differential label Jan 16, 2025

JianKangEgon and others added 3 commits January 16, 2025 15:43

feat: add new maintainer contact

cc50fc9

Signed-off-by: JianKangEgon <[email protected]>

feat: update CI build depends

1aaaac4

Signed-off-by: JianKangEgon <[email protected]>

style(pre-commit): autofix

2056acc

JianKangEgon mentioned this pull request Jan 16, 2025

Port 'autoware_vehicle_info_utils' and 'autoware_universe_utils' from Autoware Universe to Autoware Core autowarefoundation/autoware.core#131

Open

25 tasks

JianKangEgon and others added 4 commits January 16, 2025 18:30

feat: fix compile error

a6db35c

Signed-off-by: JianKangEgon <[email protected]>

style(pre-commit): autofix

a646c83

fix: maintainer name

937c46f

Signed-off-by: JianKangEgon <[email protected]>

Merge branch 'main' of github.com:JianKangEgon/autoware_utils into main

a28b3eb

NorahXiong force-pushed the main branch from 3f12ae2 to 1204a19 Compare January 17, 2025 05:25

fix(autoware_utils): duplicate variable name and function name in the…

0bea3f7

… same namespace will cause compile error when triggering template specilization Signed-off-by: NorahXiong <[email protected]>

NorahXiong force-pushed the main branch from 1204a19 to 0bea3f7 Compare January 17, 2025 05:27

JianKangEgon mentioned this pull request Jan 17, 2025

feat(autoware_vehicle_info_utils): porting from universe to core autowarefoundation/autoware.core#170

Closed

JianKangEgon and others added 3 commits January 17, 2025 14:12

fix: spell check error

eb4f047

Signed-off-by: JianKangEgon <[email protected]>

Merge branch 'main' of github.com:JianKangEgon/autoware_utils into main

8454d9c

style(pre-commit): autofix

0958b25

This was referenced Jan 17, 2025

feat: port autoware_motion_utils from autoware.universe autowarefoundation/autoware.core#171

Closed

feat: port autoware_interpolation from autoware.universe autowarefoundation/autoware.core#149

Open

JianKangEgon mentioned this pull request Jan 18, 2025

feat(autoware_test_utils): porting autoware_test_utils from universe to core autowarefoundation/autoware.core#172

Open

youtalk reviewed Jan 21, 2025

View reviewed changes

esteve requested changes Jan 21, 2025

View reviewed changes

mitsudome-r mentioned this pull request Jan 21, 2025

feat(autoware_objects_of_interest_marker_interface): porting from universe to core autowarefoundation/autoware.core#168

Open

youtalk reviewed Jan 22, 2025

View reviewed changes

JianKangEgon and others added 2 commits January 22, 2025 13:20

feat: rename namaspace autoware_utils to autoware::utils

a5d7b60

Signed-off-by: JianKangEgon <[email protected]>

style(pre-commit): autofix

483ad63

feat: revert namespace back to autoware_utils, new namespace will be …

c04c422

…updated in further version Signed-off-by: JianKangEgon <[email protected]>

liuXinGangChina requested review from youtalk and esteve January 22, 2025 08:29

youtalk changed the title ~~feat(autoware_utils): porting from universe to core~~ feat(autoware_utils): porting from autoware_universe_utils Jan 23, 2025

youtalk approved these changes Jan 23, 2025

View reviewed changes

mitsudome-r approved these changes Jan 23, 2025

View reviewed changes

esteve approved these changes Jan 23, 2025

View reviewed changes

mitsudome-r merged commit 182c967 into autowarefoundation:main Jan 23, 2025
14 of 15 checks passed

This was referenced Feb 5, 2025

feat(autoware_ekf_localizer)!: porting from universe to core 2nd autowarefoundation/autoware.universe#10067

Open

feat(autoware_ekf_localizer)!: porting from universe to core 2nd autowarefoundation/autoware.core#180

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(autoware_utils): porting from autoware_universe_utils #23

feat(autoware_utils): porting from autoware_universe_utils #23

JianKangEgon commented Jan 16, 2025 •

edited by liuXinGangChina

Loading

github-actions bot commented Jan 16, 2025 •

edited

Loading

youtalk left a comment

mitsudome-r commented Jan 21, 2025

mitsudome-r commented Jan 21, 2025

esteve left a comment

esteve Jan 21, 2025

esteve Jan 21, 2025

esteve Jan 21, 2025

esteve Jan 21, 2025

esteve Jan 21, 2025

esteve Jan 21, 2025

esteve Jan 21, 2025

liuXinGangChina Jan 22, 2025

esteve Jan 21, 2025

esteve Jan 21, 2025

esteve Jan 21, 2025

youtalk Jan 22, 2025

youtalk Jan 22, 2025

liuXinGangChina Jan 22, 2025

mitsudome-r commented Jan 22, 2025 •

edited

Loading

liuXinGangChina commented Jan 22, 2025

liuXinGangChina commented Jan 22, 2025 •

edited

Loading

esteve commented Jan 22, 2025

liuXinGangChina commented Jan 23, 2025 •

edited

Loading

youtalk left a comment

youtalk commented Jan 23, 2025

mitsudome-r commented Jan 23, 2025

mitsudome-r left a comment

esteve commented Jan 23, 2025

esteve commented Jan 23, 2025 •

edited

Loading

esteve left a comment


		#include <geometry_msgs/msg/point.hpp>

		namespace autoware_utils


		#include <autoware_utils/geometry/geometry.hpp>

		namespace autoware_utils


		#include "autoware_utils/geometry/boost_geometry.hpp"

		namespace autoware_utils::sat

	} // namespace autoware_utils::sat
	} // namespace autoware::utils::sat

feat(autoware_utils): porting from autoware_universe_utils #23

feat(autoware_utils): porting from autoware_universe_utils #23

Conversation

JianKangEgon commented Jan 16, 2025 • edited by liuXinGangChina Loading

Description

How was this PR tested?

Notes for reviewers

Effects on system behavior

github-actions bot commented Jan 16, 2025 • edited Loading

youtalk left a comment

Choose a reason for hiding this comment

mitsudome-r commented Jan 21, 2025

mitsudome-r commented Jan 21, 2025

esteve left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitsudome-r commented Jan 22, 2025 • edited Loading

liuXinGangChina commented Jan 22, 2025

liuXinGangChina commented Jan 22, 2025 • edited Loading

esteve commented Jan 22, 2025

liuXinGangChina commented Jan 23, 2025 • edited Loading

youtalk left a comment

Choose a reason for hiding this comment

youtalk commented Jan 23, 2025

mitsudome-r commented Jan 23, 2025

mitsudome-r left a comment

Choose a reason for hiding this comment

esteve commented Jan 23, 2025

esteve commented Jan 23, 2025 • edited Loading

esteve left a comment

Choose a reason for hiding this comment

JianKangEgon commented Jan 16, 2025 •

edited by liuXinGangChina

Loading

github-actions bot commented Jan 16, 2025 •

edited

Loading

mitsudome-r commented Jan 22, 2025 •

edited

Loading

liuXinGangChina commented Jan 22, 2025 •

edited

Loading

liuXinGangChina commented Jan 23, 2025 •

edited

Loading

esteve commented Jan 23, 2025 •

edited

Loading